Skip to content

refactor: custom pricing refactor#2178

Closed
Pratham-Mishra04 wants to merge 1 commit intographite-base/2178from
03-19-refactor_custom_pricing_refactor
Closed

refactor: custom pricing refactor#2178
Pratham-Mishra04 wants to merge 1 commit intographite-base/2178from
03-19-refactor_custom_pricing_refactor

Conversation

@Pratham-Mishra04
Copy link
Copy Markdown
Collaborator

Summary

Briefly explain the purpose of this PR and the problem it solves.

Changes

  • What was changed and why
  • Any notable design decisions or trade-offs

Type of change

  • Bug fix
  • Feature
  • Refactor
  • Documentation
  • Chore/CI

Affected areas

  • Core (Go)
  • Transports (HTTP)
  • Providers/Integrations
  • Plugins
  • UI (Next.js)
  • Docs

How to test

Describe the steps to validate this change. Include commands and expected outcomes.

# Core/Transports
go version
go test ./...

# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build

If adding new configs or environment variables, document them here.

Screenshots/Recordings

If UI changes, add before/after screenshots or short clips.

Breaking changes

  • Yes
  • No

If yes, describe impact and migration instructions.

Related issues

Link related issues and discussions. Example: Closes #123

Security considerations

Note any security implications (auth, secrets, PII, sandboxing, etc.).

Checklist

  • I read docs/contributing/README.md and followed the guidelines
  • I added/updated tests where appropriate
  • I updated documentation where needed
  • I verified builds succeed (Go and UI)
  • I verified the CI pipeline passes locally if applicable

Copy link
Copy Markdown
Collaborator Author

Pratham-Mishra04 commented Mar 19, 2026

This stack of pull requests is managed by Graphite. Learn more about stacking.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 19, 2026

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Added pricing field selector component with searchable, grouped pricing field interface.
    • Added empty-state UI for pricing overrides with documentation link and quick-create action.
    • Pricing overrides now supported in configuration files with schema definitions.
  • Refactor

    • Redesigned pricing override system: simplified scope and match type handling; changed update semantics to full-body replacement instead of partial patches.
    • Updated HTTP endpoint for pricing override updates from PATCH to PUT.

Walkthrough

This PR refactors pricing overrides from a strongly-typed governance package to a database-first model with string-based enums. It removes the pricingoverrides package, consolidates override handling into ModelCatalog, updates HTTP endpoints from PATCH to PUT semantics, and modifies UI components for pricing override management.

Changes

Cohort / File(s) Summary
Dependency Updates
cli/go.mod
Updated indirect golang.org/x/exp dependency version.
Governance Config & Store Layer
framework/configstore/clientconfig.go, framework/configstore/store.go
Added GeneratePricingOverrideHash function and PricingOverrides field to GovernanceConfig. Replaced PricingOverrideFilter struct with PricingOverrideFilters (changing ScopeKind from typed enum to string), removing pricingoverrides package dependency.
Database Migrations
framework/configstore/migrations.go, framework/configstore/migrations_test.go
Removed pricing_overrides_json from migration column selection. Deleted test for pricing override table reconciliation migration.
RDB Store Implementation
framework/configstore/rdb.go
Updated GetPricingOverrides method signature to accept PricingOverrideFilters instead of PricingOverrideFilter; adjusted query building to reference plural filters.* fields.
Table Definitions
framework/configstore/tables/pricingoverride.go
Changed ScopeKind and MatchType from typed enums to strings. Removed Patch field and conversion helpers (ToPricingOverride, TablePricingOverrideFromPricingOverride). Simplified BeforeSave/AfterFind hooks, removing validation and patch serialization logic. Updated PricingPatchJSON JSON tag to pricing_patch.
Log Store Formatting
framework/logstore/tables.go
Standardized whitespace/formatting in pointer field declarations for SearchFilters and Log structs without functional changes.
ModelCatalog Core
framework/modelcatalog/main.go, framework/modelcatalog/main_test.go
Removed pricingoverrides dependency and scopedOverrides storage. Added SetPricingOverrides, UpsertPricingOverrides, and DeletePricingOverride methods. Embedded PricingOptions into PricingEntry. Updated pricing reload paths to sync overrides from store.
Override Resolution Refactor
framework/modelcatalog/overrides.go, framework/modelcatalog/overrides_test.go
Replaced pricingoverrides-based compilation machinery with new local ScopeKind and MatchType string types. Introduced PricingOverride model with validation. Refactored override matching to use customPricingData with exact/wildcard lookups. Updated test setup to use SetPricingOverrides with PricingPatchJSON and string enums. Changed tie-breaking semantics from latest-updated-at to first-insertion wins.
Pricing Calculation
framework/modelcatalog/pricing.go, framework/modelcatalog/pricing_test.go, framework/modelcatalog/utils.go
Replaced getPricingLocked(...) with getBasePricing(...) returning value+flag. Introduced resolvePricing that applies overrides post-lookup. Updated convertTableModelPricingToPricingData to construct PricingOptions. Added convertTablePricingOverrideToPricingOverride function unmarshalling PricingPatchJSON.
Package Removal
framework/pricingoverrides/pricing_overrides.go
Deleted entire pricingoverrides package including ScopeKind/MatchType enums, Patch and Override structs, and validation/normalization helpers.
HTTP Handlers
transports/bifrost-http/handlers/governance.go, transports/bifrost-http/handlers/pricing_override_test.go
Changed endpoint from PATCH to PUT, renamed handler from patchPricingOverride to updatePricingOverride. Consolidated request models into single CreatePricingOverrideRequest for updates. Updated persistence to store PricingPatchJSON via sonic.Marshal and scope/match fields as strings. Replaced validation with modelcatalog.PricingOverride{}.IsValid(). Added direct ModelCatalog upsert/delete calls.
Config Application
transports/bifrost-http/lib/config.go, transports/bifrost-http/lib/config_test.go
Added pricing override reconciliation with hash-based conflict detection. Apply overrides to ModelCatalog when store unavailable. Updated MockConfigStore.GetPricingOverrides signature. Extended store transactions to create/update pricing overrides and batch upsert into catalog.
Schema & Configuration
transports/config.schema.json
Added pricing_overrides array property to governance config schema. Defined $defs for pricing_override and provider_pricing_override with validation for scope kinds, match types, and cost fields.
UI Components - Pricing Overrides
ui/app/workspace/custom-pricing/overrides/pricingFieldSelector.tsx, ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx, ui/app/workspace/custom-pricing/overrides/pricingOverridesEmptyState.tsx, ui/app/workspace/custom-pricing/overrides/scopedPricingOverridesView.tsx
Added PricingFieldSelector component for searchable, grouped pricing field selection. Added PricingOverridesEmptyState component. Refactored drawer to use new field selector, removed patch-building validation, integrated useUpdatePricingOverrideMutation. Added empty-state rendering path in scoped view.
UI Navigation
ui/components/sidebar.tsx
Capitalized "Pricing Overrides" label for Models subitem. Minor formatting adjustments to className template strings.
UI API Integration
ui/lib/store/apis/governanceApi.ts
Renamed patchPricingOverride mutation to updatePricingOverride with PUT HTTP method. Updated request type to { id: string; data: CreatePricingOverrideRequest }. Renamed exported hook from usePatchPricingOverrideMutation to useUpdatePricingOverrideMutation.
UI Type Definitions
ui/lib/types/governance.ts
Deleted PatchPricingOverrideRequest interface. Updated PricingOverridePatch with new cost fields (batches, cache-related, image-quality variants) and removed character-based and redundant audio/video fields.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 From typed enums to strings so free,
Pricing overrides dance with glee!
No more packages in the way,
ModelCatalog leads the play.
Hash-based changes, PUT instead of PATCH—
A refactored future, the perfect match! ✨

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 03-19-refactor_custom_pricing_refactor
📝 Coding Plan
  • Generate coding plan for human review comments

Comment @coderabbitai help to get the list of available commands and usage tips.

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Pratham-Mishra04 Pratham-Mishra04 force-pushed the 03-19-refactor_custom_pricing_refactor branch from d105ab7 to 964f05c Compare March 19, 2026 15:27
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
framework/modelcatalog/overrides_test.go (1)

61-97: ⚠️ Potential issue | 🟡 Minor

Add explanatory comments for seven skipped tests.

Seven tests in overrides_test.go are skipped without explanation (t.Skip()). Add TODO comments to each indicating when they should be unskipped and what is preventing them from running (e.g., incomplete feature, blocking issue). This improves maintainability and clarifies feature status.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@framework/modelcatalog/overrides_test.go` around lines 61 - 97, In
TestGetPricing_RequestTypeSpecificOverrideBeatsGeneric (and the six other
skipped tests in overrides_test.go) replace the bare t.Skip() with a brief TODO
comment immediately above the t.Skip() that states why the test is skipped
(e.g., "blocking: feature X not implemented" or a linked issue/PR number) and
when it should be unskipped (e.g., "unskip when PR `#123` implements ..."); ensure
the comment references the test name so it’s easy to find and maintain, and keep
the t.Skip() call so behavior is unchanged until the blocking work is resolved.
framework/modelcatalog/overrides.go (1)

352-416: ⚠️ Potential issue | 🟠 Major

Add missing image pricing tier fields to patchPricing.

The function is missing OutputCostPerImageAbove2048x2048Pixels and OutputCostPerImageAbove4096x4096Pixels from the loop (defined in PricingOptions at lines 110-111 in main.go). These overrides will be silently ignored without being included in the patch logic.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@framework/modelcatalog/overrides.go` around lines 352 - 416, The patchPricing
function is missing handling for OutputCostPerImageAbove2048x2048Pixels and
OutputCostPerImageAbove4096x4096Pixels so those overrides are ignored; update
patchPricing to include entries that map
patched.OutputCostPerImageAbove2048x2048Pixels and
patched.OutputCostPerImageAbove4096x4096Pixels to
override.OutputCostPerImageAbove2048x2048Pixels and
override.OutputCostPerImageAbove4096x4096Pixels respectively in the loop that
sets optional *float64 fields (the same pattern used for
OutputCostPerImageAbove1024x1024Pixels etc.) so the overrides are applied.
🧹 Nitpick comments (5)
ui/app/workspace/custom-pricing/overrides/scopedPricingOverridesView.tsx (1)

194-206: Deduplicate PricingOverrideDrawer rendering to avoid prop drift across branches.

The same drawer config now exists in both the empty-state early return and the main layout path.

♻️ Suggested consolidation
+	const drawer = (
+		<PricingOverrideDrawer
+			open={isDrawerOpen}
+			onOpenChange={setIsDrawerOpen}
+			editingOverride={editingOverride}
+			scopeLock={createScopeLock}
+		/>
+	);

 	if (!isLoading && !error && rows.length === 0) {
 		return (
 			<>
 				<PricingOverridesEmptyState onCreateClick={openCreateDrawer} />
-				<PricingOverrideDrawer
-					open={isDrawerOpen}
-					onOpenChange={setIsDrawerOpen}
-					editingOverride={editingOverride}
-					scopeLock={createScopeLock}
-				/>
+				{drawer}
 			</>
 		);
 	}

 	return (
 		<div className="mt-6 space-y-4">
 			...
-			<PricingOverrideDrawer
-				open={isDrawerOpen}
-				onOpenChange={setIsDrawerOpen}
-				editingOverride={editingOverride}
-				scopeLock={createScopeLock}
-			/>
+			{drawer}
 			...
 		</div>
 	);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ui/app/workspace/custom-pricing/overrides/scopedPricingOverridesView.tsx`
around lines 194 - 206, The PricingOverrideDrawer is duplicated in the
empty-state early return and the main render, which can cause prop drift;
refactor so the drawer is rendered once outside the conditional branch: keep the
existing empty-state return that renders PricingOverridesEmptyState (using
openCreateDrawer) but remove the inline PricingOverrideDrawer there and instead
add a single PricingOverrideDrawer in the common render path (using
isDrawerOpen, setIsDrawerOpen, editingOverride, createScopeLock) so the drawer
props are managed in one place and not duplicated across the
isLoading/error/rows.length branches.
transports/config.schema.json (1)

3105-3111: Consider referencing the enum definition for request_types items.

The request_types array items are defined as plain "type": "string" rather than referencing the pricing_override_request_type enum defined at lines 3128-3143. This means the schema won't validate that values are from the allowed set.

🔧 Suggested fix
         "request_types": {
           "type": "array",
           "description": "Request types this override applies to (empty = all types)",
           "items": {
-            "type": "string"
+            "$ref": "#/$defs/pricing_override_request_type"
           }
         },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@transports/config.schema.json` around lines 3105 - 3111, The request_types
array currently uses a generic string item type and should instead validate
against the existing enum; update the request_types schema (the "request_types"
property) so its "items" uses a $ref to the pricing_override_request_type enum
definition (referencing the pricing_override_request_type schema name) rather
than "type": "string", ensuring values are validated against the allowed set.
framework/modelcatalog/overrides_test.go (1)

36-54: Consider using bifrost.Ptr() for pointer creation.

The test creates pointers using the address operator (&providerID). Based on learnings, the repository prefers using bifrost.Ptr() for consistency across all code paths, including test utilities.

♻️ Suggested refactor
-	providerID := "openai"
-	require.NoError(t, mc.SetPricingOverrides([]configstoreTables.TablePricingOverride{
+	require.NoError(t, mc.SetPricingOverrides([]configstoreTables.TablePricingOverride{
 		{
 			ID:               "openai-override-0",
 			ScopeKind:        string(ScopeKindProvider),
-			ProviderID:       &providerID,
+			ProviderID:       bifrost.Ptr("openai"),
 			MatchType:        string(MatchTypeWildcard),
 			Pattern:          "gpt-*",
 			PricingPatchJSON: `{"input_cost_per_token":10}`,
 		},
 		{
 			ID:               "openai-override-1",
 			ScopeKind:        string(ScopeKindProvider),
-			ProviderID:       &providerID,
+			ProviderID:       bifrost.Ptr("openai"),
 			MatchType:        string(MatchTypeExact),
 			Pattern:          "gpt-4o",
 			PricingPatchJSON: `{"input_cost_per_token":20}`,
 		},
 	}))

This pattern should be applied consistently throughout the test file. Based on learnings: "prefer using bifrost.Ptr() to create pointers instead of the address operator (&) even when & would be valid syntactically."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@framework/modelcatalog/overrides_test.go` around lines 36 - 54, Replace uses
of the address operator for providerID in the test's TablePricingOverride
entries with bifrost.Ptr(...) for consistency: in the block where providerID is
defined and passed into mc.SetPricingOverrides (the TablePricingOverride structs
used with SetPricingOverrides), change all instances of &providerID to
bifrost.Ptr(providerID); apply the same replacement consistently across this
test file wherever plain &-style pointer creation is used.
ui/app/workspace/custom-pricing/overrides/pricingFieldSelector.tsx (1)

36-47: Consider optimizing the auto-activation effect.

The useEffect creates a new Set on every invocation even when no fields need to be added. Consider checking if any additions are needed before creating the new Set:

♻️ Suggested optimization
 	useEffect(() => {
 		setActiveFields((prev) => {
+			let needsUpdate = false;
+			for (const f of PRICING_FIELDS) {
+				if (values[f.key] != null && values[f.key]!.trim() !== "" && !prev.has(f.key)) {
+					needsUpdate = true;
+					break;
+				}
+			}
+			if (!needsUpdate) return prev;
+
 			const next = new Set(prev);
 			for (const f of PRICING_FIELDS) {
 				if (values[f.key] != null && values[f.key]!.trim() !== "") {
 					next.add(f.key);
 				}
 			}
 			return next;
 		});
 	}, [values]);

This avoids creating a new Set reference when no fields need activation, preventing unnecessary re-renders of child components.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ui/app/workspace/custom-pricing/overrides/pricingFieldSelector.tsx` around
lines 36 - 47, The useEffect auto-activation currently always constructs a new
Set and calls setActiveFields causing unnecessary re-renders; modify the effect
that references useEffect, PRICING_FIELDS, values and setActiveFields to first
scan PRICING_FIELDS for any keys where values[key] is non-empty and not already
present in the current prev set, and only when at least one such key exists
create a new Set based on prev, add the missing keys and call
setActiveFields(next); otherwise do nothing (avoid creating the Set or calling
setActiveFields).
framework/modelcatalog/main.go (1)

258-261: Consider failing initialization if pricing overrides fail to load.

Currently, a failure to load pricing overrides only logs a warning but allows initialization to continue. If pricing overrides are critical to correct billing behavior, this might lead to silent billing inaccuracies.

Depending on business requirements, consider whether this should be a fatal error:

 	if err := mc.loadPricingOverridesFromStore(ctx); err != nil {
-		logger.Warn("failed to load pricing overrides: %v", err)
+		return nil, fmt.Errorf("failed to load pricing overrides: %w", err)
 	}

Or at minimum, ensure there's monitoring/alerting for this warning path.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@framework/modelcatalog/main.go` around lines 258 - 261, The pricing overrides
load is currently treated as a warning; change initialization to fail fast by
returning the error from the initializer instead of calling logger.Warn when
mc.loadPricingOverridesFromStore(ctx) returns non-nil. Locate the call to
mc.loadPricingOverridesFromStore in the initializer (the block using
logger.Warn) and replace the warn-only behavior with a wrapped error return (or
propagate the original error) so the caller cannot continue with a possibly
incorrect billing state; optionally log the error just before returning for
context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 3343-3348: When h.modelCatalog.UpsertPricingOverrides(&override)
returns an error you currently log and call SendError but then continue
execution and later send a 201; stop execution after the in-memory upsert
failure by returning from the HTTP handler immediately after the SendError call.
Specifically, update the block that calls h.modelCatalog.UpsertPricingOverrides
to: on err, call logger.Error(...), call SendError(ctx,
fasthttp.StatusInternalServerError, ...), then return from the surrounding
handler function so no 201 Created is emitted.
- Around line 3394-3407: The PR resets ConfigHash to an empty string when
constructing the updated configstoreTables.TablePricingOverride in the PUT
handler, which breaks hash-based reconciliation; instead preserve the existing
ConfigHash (e.g., use existing.ConfigHash) or only clear it when intentionally
recomputing the hash elsewhere. Update the override construction in the PUT path
(the TablePricingOverride build) to keep the prior ConfigHash rather than
assigning "" so file-origin overrides don't appear modified on restart.

In `@transports/bifrost-http/lib/config.go`:
- Around line 1187-1193: The loop that sets ConfigHash for each item in
configData.Governance.PricingOverrides calls
configstore.GeneratePricingOverrideHash before serializing RequestTypes into the
RequestTypesJSON field, so changes to request_types are ignored; update the loop
to serialize/normalize the override.RequestTypes into override.RequestTypesJSON
(same normalization used in createGovernanceConfigInStore) prior to calling
GeneratePricingOverrideHash, and apply the identical fix to the other occurrence
(the createGovernanceConfigInStore code path) so both places compute the hash
from the serialized RequestTypesJSON.
- Around line 940-945: loadGovernanceConfigFromFile applies pricing overrides
too early when config.ModelCatalog is still nil, so moves are dropped; after
initFrameworkConfigFromFile completes (where config.ModelCatalog is initialized)
add a check for config.ConfigStore == nil && config.ModelCatalog != nil &&
config.GovernanceConfig != nil && len(config.GovernanceConfig.PricingOverrides)
> 0 and call
config.ModelCatalog.SetPricingOverrides(config.GovernanceConfig.PricingOverrides),
logging any error with the existing logger.Warn pattern; reference the functions
loadGovernanceConfigFromFile and initFrameworkConfigFromFile and the symbols
config.ModelCatalog, config.ConfigStore,
config.GovernanceConfig.PricingOverrides, and ModelCatalog.SetPricingOverrides
when locating where to insert this replay logic.

In `@transports/config.schema.json`:
- Around line 3128-3143: The JSON schema property pricing_override_request_type
is missing three request types present elsewhere (mcp_tool_execution,
websocket_responses, realtime) causing validation mismatch; update the
pricing_override_request_type enum to include "mcp_tool_execution",
"websocket_responses", and "realtime" so it matches the definitions in
core/schemas/bifrost.go and ui/lib/types/config.ts and ensures the
frontend-accepted request types validate against this schema.

---

Outside diff comments:
In `@framework/modelcatalog/overrides_test.go`:
- Around line 61-97: In TestGetPricing_RequestTypeSpecificOverrideBeatsGeneric
(and the six other skipped tests in overrides_test.go) replace the bare t.Skip()
with a brief TODO comment immediately above the t.Skip() that states why the
test is skipped (e.g., "blocking: feature X not implemented" or a linked
issue/PR number) and when it should be unskipped (e.g., "unskip when PR `#123`
implements ..."); ensure the comment references the test name so it’s easy to
find and maintain, and keep the t.Skip() call so behavior is unchanged until the
blocking work is resolved.

In `@framework/modelcatalog/overrides.go`:
- Around line 352-416: The patchPricing function is missing handling for
OutputCostPerImageAbove2048x2048Pixels and
OutputCostPerImageAbove4096x4096Pixels so those overrides are ignored; update
patchPricing to include entries that map
patched.OutputCostPerImageAbove2048x2048Pixels and
patched.OutputCostPerImageAbove4096x4096Pixels to
override.OutputCostPerImageAbove2048x2048Pixels and
override.OutputCostPerImageAbove4096x4096Pixels respectively in the loop that
sets optional *float64 fields (the same pattern used for
OutputCostPerImageAbove1024x1024Pixels etc.) so the overrides are applied.

---

Nitpick comments:
In `@framework/modelcatalog/main.go`:
- Around line 258-261: The pricing overrides load is currently treated as a
warning; change initialization to fail fast by returning the error from the
initializer instead of calling logger.Warn when
mc.loadPricingOverridesFromStore(ctx) returns non-nil. Locate the call to
mc.loadPricingOverridesFromStore in the initializer (the block using
logger.Warn) and replace the warn-only behavior with a wrapped error return (or
propagate the original error) so the caller cannot continue with a possibly
incorrect billing state; optionally log the error just before returning for
context.

In `@framework/modelcatalog/overrides_test.go`:
- Around line 36-54: Replace uses of the address operator for providerID in the
test's TablePricingOverride entries with bifrost.Ptr(...) for consistency: in
the block where providerID is defined and passed into mc.SetPricingOverrides
(the TablePricingOverride structs used with SetPricingOverrides), change all
instances of &providerID to bifrost.Ptr(providerID); apply the same replacement
consistently across this test file wherever plain &-style pointer creation is
used.

In `@transports/config.schema.json`:
- Around line 3105-3111: The request_types array currently uses a generic string
item type and should instead validate against the existing enum; update the
request_types schema (the "request_types" property) so its "items" uses a $ref
to the pricing_override_request_type enum definition (referencing the
pricing_override_request_type schema name) rather than "type": "string",
ensuring values are validated against the allowed set.

In `@ui/app/workspace/custom-pricing/overrides/pricingFieldSelector.tsx`:
- Around line 36-47: The useEffect auto-activation currently always constructs a
new Set and calls setActiveFields causing unnecessary re-renders; modify the
effect that references useEffect, PRICING_FIELDS, values and setActiveFields to
first scan PRICING_FIELDS for any keys where values[key] is non-empty and not
already present in the current prev set, and only when at least one such key
exists create a new Set based on prev, add the missing keys and call
setActiveFields(next); otherwise do nothing (avoid creating the Set or calling
setActiveFields).

In `@ui/app/workspace/custom-pricing/overrides/scopedPricingOverridesView.tsx`:
- Around line 194-206: The PricingOverrideDrawer is duplicated in the
empty-state early return and the main render, which can cause prop drift;
refactor so the drawer is rendered once outside the conditional branch: keep the
existing empty-state return that renders PricingOverridesEmptyState (using
openCreateDrawer) but remove the inline PricingOverrideDrawer there and instead
add a single PricingOverrideDrawer in the common render path (using
isDrawerOpen, setIsDrawerOpen, editingOverride, createScopeLock) so the drawer
props are managed in one place and not duplicated across the
isLoading/error/rows.length branches.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 859c97a9-85cf-4135-a04f-4a534653891a

📥 Commits

Reviewing files that changed from the base of the PR and between b8939bb and 964f05c.

⛔ Files ignored due to path filters (1)
  • cli/go.sum is excluded by !**/*.sum
📒 Files selected for processing (28)
  • cli/go.mod
  • framework/configstore/clientconfig.go
  • framework/configstore/migrations.go
  • framework/configstore/migrations_test.go
  • framework/configstore/rdb.go
  • framework/configstore/store.go
  • framework/configstore/tables/pricingoverride.go
  • framework/logstore/tables.go
  • framework/modelcatalog/main.go
  • framework/modelcatalog/main_test.go
  • framework/modelcatalog/overrides.go
  • framework/modelcatalog/overrides_test.go
  • framework/modelcatalog/pricing.go
  • framework/modelcatalog/pricing_test.go
  • framework/modelcatalog/utils.go
  • framework/pricingoverrides/pricing_overrides.go
  • transports/bifrost-http/handlers/governance.go
  • transports/bifrost-http/handlers/pricing_override_test.go
  • transports/bifrost-http/lib/config.go
  • transports/bifrost-http/lib/config_test.go
  • transports/config.schema.json
  • ui/app/workspace/custom-pricing/overrides/pricingFieldSelector.tsx
  • ui/app/workspace/custom-pricing/overrides/pricingOverrideDrawer.tsx
  • ui/app/workspace/custom-pricing/overrides/pricingOverridesEmptyState.tsx
  • ui/app/workspace/custom-pricing/overrides/scopedPricingOverridesView.tsx
  • ui/components/sidebar.tsx
  • ui/lib/store/apis/governanceApi.ts
  • ui/lib/types/governance.ts
💤 Files with no reviewable changes (4)
  • framework/modelcatalog/main_test.go
  • framework/configstore/migrations_test.go
  • framework/configstore/migrations.go
  • framework/pricingoverrides/pricing_overrides.go
👮 Files not reviewed due to content moderation or server errors (6)
  • framework/configstore/rdb.go
  • framework/modelcatalog/utils.go
  • ui/app/workspace/custom-pricing/overrides/pricingOverridesEmptyState.tsx
  • framework/configstore/clientconfig.go
  • ui/lib/store/apis/governanceApi.ts
  • transports/bifrost-http/handlers/pricing_override_test.go

Comment on lines +3343 to +3348
if h.modelCatalog != nil {
if err := h.modelCatalog.UpsertPricingOverrides(&override); err != nil {
logger.Error("failed to upsert pricing override: %v", err)
SendError(ctx, fasthttp.StatusInternalServerError, "Failed to upsert pricing override")
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Stop after the in-memory upsert failure.

If UpsertPricingOverrides fails, this sends a 500 and then still falls through to the 201 Created response below. That reports success while pricing is stale in memory.

💡 Suggested fix
 if h.modelCatalog != nil {
 	if err := h.modelCatalog.UpsertPricingOverrides(&override); err != nil {
 		logger.Error("failed to upsert pricing override: %v", err)
 		SendError(ctx, fasthttp.StatusInternalServerError, "Failed to upsert pricing override")
+		return
 	}
 }
Based on learnings: In the Bifrost repository, when updating routing rules or governance entities via the HTTP handler in `transports/bifrost-http/handlers/governance.go`, if the database update succeeds but the in-memory GovernanceManager reload fails, respond with HTTP 500 to the client rather than signaling success.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if h.modelCatalog != nil {
if err := h.modelCatalog.UpsertPricingOverrides(&override); err != nil {
logger.Error("failed to upsert pricing override: %v", err)
SendError(ctx, fasthttp.StatusInternalServerError, "Failed to upsert pricing override")
}
}
if h.modelCatalog != nil {
if err := h.modelCatalog.UpsertPricingOverrides(&override); err != nil {
logger.Error("failed to upsert pricing override: %v", err)
SendError(ctx, fasthttp.StatusInternalServerError, "Failed to upsert pricing override")
return
}
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@transports/bifrost-http/handlers/governance.go` around lines 3343 - 3348,
When h.modelCatalog.UpsertPricingOverrides(&override) returns an error you
currently log and call SendError but then continue execution and later send a
201; stop execution after the in-memory upsert failure by returning from the
HTTP handler immediately after the SendError call. Specifically, update the
block that calls h.modelCatalog.UpsertPricingOverrides to: on err, call
logger.Error(...), call SendError(ctx, fasthttp.StatusInternalServerError, ...),
then return from the surrounding handler function so no 201 Created is emitted.

Comment on lines +3394 to +3407
override := configstoreTables.TablePricingOverride{
ID: id,
Name: name,
ScopeKind: string(req.ScopeKind),
VirtualKeyID: normalizeOptionalString(req.VirtualKeyID),
ProviderID: normalizeOptionalString(req.ProviderID),
ProviderKeyID: normalizeOptionalString(req.ProviderKeyID),
MatchType: string(req.MatchType),
Pattern: strings.TrimSpace(req.Pattern),
RequestTypes: req.RequestTypes,
PricingPatchJSON: string(patchJSON),
ConfigHash: "",
CreatedAt: existing.CreatedAt,
UpdatedAt: time.Now(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't clear ConfigHash on PUT updates.

Resetting ConfigHash to "" here breaks the hash-based reconciliation in loadGovernanceConfigFromFile. Overrides that originally came from config.json will look modified from file on every restart, so dashboard edits get overwritten.

💡 Suggested fix
-		ConfigHash:       "",
+		ConfigHash:       existing.ConfigHash,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
override := configstoreTables.TablePricingOverride{
ID: id,
Name: name,
ScopeKind: string(req.ScopeKind),
VirtualKeyID: normalizeOptionalString(req.VirtualKeyID),
ProviderID: normalizeOptionalString(req.ProviderID),
ProviderKeyID: normalizeOptionalString(req.ProviderKeyID),
MatchType: string(req.MatchType),
Pattern: strings.TrimSpace(req.Pattern),
RequestTypes: req.RequestTypes,
PricingPatchJSON: string(patchJSON),
ConfigHash: "",
CreatedAt: existing.CreatedAt,
UpdatedAt: time.Now(),
override := configstoreTables.TablePricingOverride{
ID: id,
Name: name,
ScopeKind: string(req.ScopeKind),
VirtualKeyID: normalizeOptionalString(req.VirtualKeyID),
ProviderID: normalizeOptionalString(req.ProviderID),
ProviderKeyID: normalizeOptionalString(req.ProviderKeyID),
MatchType: string(req.MatchType),
Pattern: strings.TrimSpace(req.Pattern),
RequestTypes: req.RequestTypes,
PricingPatchJSON: string(patchJSON),
ConfigHash: existing.ConfigHash,
CreatedAt: existing.CreatedAt,
UpdatedAt: time.Now(),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@transports/bifrost-http/handlers/governance.go` around lines 3394 - 3407, The
PR resets ConfigHash to an empty string when constructing the updated
configstoreTables.TablePricingOverride in the PUT handler, which breaks
hash-based reconciliation; instead preserve the existing ConfigHash (e.g., use
existing.ConfigHash) or only clear it when intentionally recomputing the hash
elsewhere. Update the override construction in the PUT path (the
TablePricingOverride build) to keep the prior ConfigHash rather than assigning
"" so file-origin overrides don't appear modified on restart.

Comment on lines +940 to +945
// No config store: load pricing overrides directly into the model catalog
if config.ConfigStore == nil && config.ModelCatalog != nil && len(configData.Governance.PricingOverrides) > 0 {
if err := config.ModelCatalog.SetPricingOverrides(configData.Governance.PricingOverrides); err != nil {
logger.Warn("failed to set pricing overrides from config file: %v", err)
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

File-only mode drops pricing overrides during boot.

loadGovernanceConfigFromFile runs before initFrameworkConfigFromFile, so config.ModelCatalog is still nil here. In the ConfigStore == nil path, pricing_overrides from config.json are never applied because nothing replays them after the catalog is initialized.

💡 Suggested placement
// after initFrameworkConfigFromFile(...)
if config.ConfigStore == nil && config.ModelCatalog != nil &&
	config.GovernanceConfig != nil && len(config.GovernanceConfig.PricingOverrides) > 0 {
	if err := config.ModelCatalog.SetPricingOverrides(config.GovernanceConfig.PricingOverrides); err != nil {
		logger.Warn("failed to set pricing overrides from config file: %v", err)
	}
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@transports/bifrost-http/lib/config.go` around lines 940 - 945,
loadGovernanceConfigFromFile applies pricing overrides too early when
config.ModelCatalog is still nil, so moves are dropped; after
initFrameworkConfigFromFile completes (where config.ModelCatalog is initialized)
add a check for config.ConfigStore == nil && config.ModelCatalog != nil &&
config.GovernanceConfig != nil && len(config.GovernanceConfig.PricingOverrides)
> 0 and call
config.ModelCatalog.SetPricingOverrides(config.GovernanceConfig.PricingOverrides),
logging any error with the existing logger.Warn pattern; reference the functions
loadGovernanceConfigFromFile and initFrameworkConfigFromFile and the symbols
config.ModelCatalog, config.ConfigStore,
config.GovernanceConfig.PricingOverrides, and ModelCatalog.SetPricingOverrides
when locating where to insert this replay logic.

Comment on lines +1187 to +1193
for i, newOverride := range configData.Governance.PricingOverrides {
fileHash, err := configstore.GeneratePricingOverrideHash(newOverride)
if err != nil {
logger.Warn("failed to generate pricing override hash for %s: %v", newOverride.ID, err)
continue
}
configData.Governance.PricingOverrides[i].ConfigHash = fileHash
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Serialize request_types before hashing pricing overrides.

GeneratePricingOverrideHash hashes RequestTypesJSON, but these paths compute the hash before serializing RequestTypes into that field. A request_types change in config.json therefore won't affect the generated hash here, so restart reconciliation can miss or misclassify updates.

💡 Suggested fix
+ requestTypesJSON, err := sonic.Marshal(configData.Governance.PricingOverrides[i].RequestTypes)
+ if err != nil {
+ 	logger.Warn("failed to marshal pricing override request types for %s: %v", newOverride.ID, err)
+ 	continue
+ }
+ configData.Governance.PricingOverrides[i].RequestTypesJSON = string(requestTypesJSON)
  fileHash, err := configstore.GeneratePricingOverrideHash(configData.Governance.PricingOverrides[i])

Apply the same normalization in createGovernanceConfigInStore(...) before hashing override.

Also applies to: 1484-1491

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@transports/bifrost-http/lib/config.go` around lines 1187 - 1193, The loop
that sets ConfigHash for each item in configData.Governance.PricingOverrides
calls configstore.GeneratePricingOverrideHash before serializing RequestTypes
into the RequestTypesJSON field, so changes to request_types are ignored; update
the loop to serialize/normalize the override.RequestTypes into
override.RequestTypesJSON (same normalization used in
createGovernanceConfigInStore) prior to calling GeneratePricingOverrideHash, and
apply the identical fix to the other occurrence (the
createGovernanceConfigInStore code path) so both places compute the hash from
the serialized RequestTypesJSON.

Comment on lines +3128 to +3143
"pricing_override_request_type": {
"type": "string",
"enum": [
"list_models", "text_completion", "text_completion_stream",
"chat_completion", "chat_completion_stream", "responses", "responses_stream",
"count_tokens", "embedding", "rerank", "speech", "speech_stream",
"transcription", "transcription_stream", "image_generation", "image_generation_stream",
"image_edit", "image_edit_stream", "image_variation", "video_generation",
"video_retrieve", "video_download", "video_delete", "video_list", "video_remix",
"batch_create", "batch_list", "batch_retrieve", "batch_cancel", "batch_delete", "batch_results",
"file_upload", "file_list", "file_retrieve", "file_delete", "file_content",
"container_create", "container_list", "container_retrieve", "container_delete",
"container_file_create", "container_file_list", "container_file_retrieve",
"container_file_content", "container_file_delete", "passthrough", "passthrough_stream"
]
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for RequestType definitions to verify the canonical source
rg -n "RequestType" --type=go -A 20 | head -100

# Check if batch_delete, passthrough types are defined in Go schemas
rg -n "batch_delete|passthrough|mcp_tool_execution|websocket_responses|realtime" --type=go

Repository: maximhq/bifrost

Length of output: 32158


🏁 Script executed:

cat ui/lib/types/config.ts | sed -n '180,227p'

Repository: maximhq/bifrost

Length of output: 1082


🏁 Script executed:

cat ui/lib/types/config.ts | sed -n '180,300p'

Repository: maximhq/bifrost

Length of output: 3204


🏁 Script executed:

cat core/schemas/bifrost.go | grep -A 50 "type RequestType string"

Repository: maximhq/bifrost

Length of output: 3040


🏁 Script executed:

cat core/schemas/bifrost.go | grep -A 60 "type RequestType string"

Repository: maximhq/bifrost

Length of output: 3393


🏁 Script executed:

git log --oneline -20

Repository: maximhq/bifrost

Length of output: 101


Add missing request types to pricing_override_request_type enum.

The enum is incomplete. It's missing mcp_tool_execution, websocket_responses, and realtime, which are defined in both the Go schemas (core/schemas/bifrost.go) and TypeScript types (ui/lib/types/config.ts). This creates a validation mismatch where the frontend can accept request types that the schema rejects. Add the three missing types to the enum.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@transports/config.schema.json` around lines 3128 - 3143, The JSON schema
property pricing_override_request_type is missing three request types present
elsewhere (mcp_tool_execution, websocket_responses, realtime) causing validation
mismatch; update the pricing_override_request_type enum to include
"mcp_tool_execution", "websocket_responses", and "realtime" so it matches the
definitions in core/schemas/bifrost.go and ui/lib/types/config.ts and ensures
the frontend-accepted request types validate against this schema.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants